Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add import sorting #612

Draft
wants to merge 1 commit into
base: main
Choose a base branch
from
Draft

Add import sorting #612

wants to merge 1 commit into from

Conversation

Abban
Copy link
Member

@Abban Abban commented Nov 8, 2024

This adds an import sorting lint rule and autofixer.
Imports will be sorted alphabetically and the local
ones moved to the bottom of the list

Ticket: https://phabricator.wikimedia.org/T378589

This adds an import sorting lint rule and autofixer.
Imports will be sorted alphabetically and the local
ones moved to the bottom of the list

Ticket: https://phabricator.wikimedia.org/T378589
Base automatically changed from timer to main November 13, 2024 15:36
Copy link
Member

@gbirke gbirke left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sorry, I can neither enthusiastically approve or disapprove here.

I do like the fact that sorting the imports is taken out of the hands of us developers who might try to impose some kind of structure to them which then fails at the next IDE autoimport. The order does not help me particularly (I caught myself expecting an alphabetical order of the import objects, not of the import paths), but the more automation the better.

Except ... This adds 2.5 seconds to the linting process, (12 vs 14.5 on my machine). OTOH that's only for a full linting run when nothing is cached, so this mostly affects CI runs which do always have an empty cache.

So the best I can do here is an "I approve, but only if no one else rejects". So please wait for another review before merging.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants